Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenColorIO : Defer expansion of context variables in OCIO vars #5898

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

johnhaddon
Copy link
Member

Previously, the OpenColorIOConfigPlug was baking in values from the current context at the point the OCIO plugs were edited on the ScriptNode. This meant that later changes to the context variable were not reflected in the OCIO variable. Among other things, this meant that the Viewer LUT would fail to update when changing shot in certain multi-shot workflows.

Moving the variable expansion to OpenColorIOAlgo::currentContextAndConfig() fixes this problem, but also enables more advanced workflows. For instance, a Wedge or ContactSheet node can now set the variable being referenced, and the OCIO config used by OpenColorIOTransform nodes will reflect that change dynamically as the graph executes.

Copy link
Contributor

@murraystevenson murraystevenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Previously, the OpenColorIOConfigPlug was baking in values from the current context at the point the OCIO plugs were edited on the ScriptNode. This meant that later changes to the context variable were not reflected in the OCIO variable. Among other things, this meant that the Viewer LUT would fail to
update when changing shot in certain multi-shot workflows.

Moving the variable expansion to `OpenColorIOAlgo::currentContextAndConfig()`
fixes this problem, but also enables more advanced workflows. For instance, a Wedge or ContactSheet node can now set the variable being referenced, and the OCIO config used by OpenColorIOTransform nodes will reflect that change dynamically as the graph executes.

There is one knock-on effect from this, as demonstrated in the test changes. On Windows, we must now use `/` separators in the context variables where before we could get away with `\`. We think it's unlikely that anyone is creating contexts manually like this in Windows - much more likely is usage of OpenColorIOContext and OpenColorIOConfigPlug where `/` has always been a requirement. So we think the very small risk of disruption is worth it for the benefits provided.
@johnhaddon
Copy link
Member Author

Squashed the fixup in, adding an explanatory comment about Windows in the commit message. Merging...

@johnhaddon johnhaddon merged commit 9a36550 into 1.4_maintenance Jun 14, 2024
5 checks passed
@johnhaddon johnhaddon deleted the ocioContextFix branch June 20, 2024 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants